Minimal type checking changes#365
Minimal type checking changes#365lispandfound wants to merge 13 commits intotesting_infra_refactorfrom
Conversation
Summary of ChangesHello @lispandfound, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the type safety and maintainability of the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
The |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces extensive type hinting throughout the qcore library, significantly improving code clarity and enabling static analysis. It also refactors several parts of the code for better readability and adherence to PEP 8 standards. While these are great improvements, the PR also includes several breaking changes by removing functions from the public API (e.g., get_fault_bb_dir, transf, calc_backarc, dump_yaml, setup_dir, compare_versions). It would be beneficial to document these removals in the pull request description. I've identified a few critical bugs in the new distribution functions and a logic issue in ExtendedEnum that need to be addressed.
There was a problem hiding this comment.
Pull request overview
This PR introduces minimal type checking infrastructure and adds type annotations throughout the qcore codebase. The changes include adding a GitHub Actions workflow for type checking, introducing function overloads for better type safety in distribution functions with size/seed parameters, refactoring deprecated functions, and removing unused code.
Key changes:
- Added type annotations to function signatures across multiple modules
- Introduced overloads for distribution functions to properly type return values based on size parameter
- Added GitHub Actions workflow for type checking using
ty - Removed deprecated/unused functions and cleaned up code
- Added
typing-extensionsdependency for better compatibility
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_xyts.py | Removed trailing blank line |
| tests/test_geo.py | Added type annotations and updated test values to use pytest.approx for floating-point comparisons; added unused Path import |
| tests/test_coordinates.py | Added type annotations to test functions; added unused re import |
| qcore/xyts.py | Added type annotations, renamed attributes (cosR→cos_r, etc.), added overloads for corners() method, added data validation checks; contains duplicate attribute definitions |
| qcore/utils.py | Deprecated load_yaml function, removed dump_yaml, setup_dir, and compare_versions functions |
| qcore/uncertainties/distributions.py | Added size and seed parameters to distribution functions with overloads for type safety; contains bugs in truncated_log_normal implementation |
| qcore/typing.py | New file with type variable definitions for numpy floating types |
| qcore/timeseries.py | Added type annotations, added type ignore for PyFFTW config, removed transf function |
| qcore/src_site_dist.py | Added type annotations and overloads for calc_rrup_rjb, removed calc_backarc function |
| qcore/siteamp_models.py | Added pragma: no cover directives for numba-compiled functions |
| qcore/simulation_structure.py | Added type annotations and simplified get_fault_yaml_path implementation |
| qcore/shared.py | Deprecated functions, changed IOBase to FileIO in type hints, added type ignore comments |
| qcore/point_in_polygon.py | Added type annotations using numpy typing |
| qcore/nhm.py | Added return type annotations |
| qcore/grid.py | Added type annotations, error handling for missing resolution parameter |
| qcore/geo.py | Updated type hints to use union syntax, improved closest_location return type |
| qcore/formats.py | Changed mutable default arguments to None, added type annotations and overloads |
| qcore/coordinates.py | Added type annotations, refactored variable names (cosA→cos_a, etc.) |
| qcore/constants.py | Added type annotations, improved is_substring safety |
| qcore/cli.py | Improved type annotations using ParamSpec and TypeVar |
| qcore/archive_structure.py | Added comprehensive docstrings and type annotations |
| pyproject.toml | Removed matplotlib dependency, added typing-extensions>=4.9.0 |
| .github/workflows/types.yml | New workflow file for type checking with ty on Python 3.13 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
station_file_argparser never actually returns None
This PR introduces minimal type checking changes into qcore, and adds a GitHub functionality to check them. I've tried to make this PR as small as possible to make it reviewable. However, the distributions module required some changes: Most functions now take a seed and size parameter. The size parameter is introduced to make the type checking work (i.e. when size = 1, distributions return floats, else they return arrays).